Skip to content

Comments

swev-id: sphinx-doc__sphinx-9320#81

Draft
casey-brooks wants to merge 2 commits intosphinx-doc__sphinx-9320from
noa/issue-75
Draft

swev-id: sphinx-doc__sphinx-9320#81
casey-brooks wants to merge 2 commits intosphinx-doc__sphinx-9320from
noa/issue-75

Conversation

@casey-brooks
Copy link

@casey-brooks casey-brooks commented Dec 26, 2025

Summary

  • allow sphinx-quickstart to accept empty response when an existing conf.py is detected and abort with a clear message
  • require a valid replacement path before continuing interactive setup
  • add regression tests covering abort, re-prompt, and quiet-mode behavior

Reproduction Steps

  1. create a directory containing an existing conf.py
  2. run sphinx-quickstart and answer prompts until it detects the conflict
  3. press Enter at the new root path prompt to observe the abort behavior

Testing

  • PYTHONPATH=/workspace:/workspace/sphinx .venv311/bin/python -m pytest tests/test_quickstart.py -q
  • PYTHONPATH=/workspace:/workspace/sphinx .venv311/bin/python -m pytest -q (fails: multiple pre-existing linkcheck, autodoc, and extension tests due to missing external tools/network and version pinning conflicts)

Fixes #75

@casey-brooks
Copy link
Author

casey-brooks commented Dec 26, 2025 •
edited
Loading

Test & Lint Summary

  • PYTHONPATH=/workspace:/workspace/sphinx .venv311/bin/python -m pytest tests/test_quickstart.py -q (pass: 13 passed, 0 failed)
  • PYTHONPATH=/workspace:/workspace/sphinx .venv311/bin/python -m pytest -q (fails: multiple pre-existing linkcheck, autodoc, and extension tests because external network access, GCC/Cython toolchain, and upstream version pins are unavailable in this environment)
  • .venv311/bin/tox -e flake8 (fails: dependency types-pkg_resources has no available distribution for this interpreter)

noa-lucent
noa-lucent requested changes Dec 26, 2025
View reviewed changes
Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tightening up the quickstart interaction and adding coverage. I left one blocking comment: the new regression test compares the entries returned by against bare strings, so it will always fail. Please convert to strings or basenames before asserting on the directory contents. Once that’s addressed I’m happy to take another pass.

noa-lucent
noa-lucent approved these changes Dec 26, 2025
View reviewed changes
Copy link

@noa-lucent noa-lucent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the follow-up. The directory-content assertion now consumes basenames, so the new regression suite passes with results. Appreciated the extra coverage—looks good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@noa-lucent noa-lucent noa-lucent approved these changes

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@casey-brooks @noa-lucent